Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)

10-31: Critical: Team main rooms deleted twice, causing potential errors.

Team main rooms undergo double deletion:

1. eraseTeam(userId, team, []) at Line 28 calls eraseRoom(team.roomId, userId) (see eraseTeam.ts Line 17), which deletes the team's main room.
2. Rooms.removeByIds(rids) at Line 48 attempts to delete the same room again, since team main rooms are included in rids.

Depending on how eraseRoom or Rooms.removeByIds handle non-existent rooms, this could throw errors or cause inconsistent state.

Solution: Exclude team main rooms from the final Rooms.removeByIds(rids) call, since they are already handled by eraseTeam.

Apply this diff:

 const bulkRoomCleanUp = async (rids: string[], userId?: string) => {
 	// no bulk deletion for files
 	await Promise.all(rids.map((rid) => FileUpload.removeFilesByRoomId(rid)));
 
+	const rooms = userId ? await Rooms.findByIds(rids, { projection: { _id: 1, teamMain: 1 } }).toArray() : [];
+	const teamMainRoomIds = new Set(rooms.filter((room) => room.teamMain).map((room) => room._id));
+
 	await Promise.all([
 		Subscriptions.removeByRoomIds(rids, {
 			async onTrash(doc) {
 				void notifyOnSubscriptionChanged(doc, 'removed');
 			},
 		}),
 		Messages.removeByRoomIds(rids),
 		ReadReceipts.removeByRoomIds(rids),
 		...[userId && bulkTeamCleanup(rids, userId)],
 	]);
 
-	await Rooms.removeByIds(rids);
+	// Exclude team main rooms as they are already deleted by eraseTeam
+	const roomsToDelete = rids.filter((rid) => !teamMainRoomIds.has(rid));
+	if (roomsToDelete.length > 0) {
+		await Rooms.removeByIds(roomsToDelete);
+	}
 };

apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (2)

19-21: Move userId validation before Promise.all for efficiency.

The userId validation is executed inside the map callback for every team ID. Since userId is constant across all iterations, validate it once before entering Promise.all to avoid redundant checks.

Apply this diff:

 const bulkTeamCleanup = async (rids: IRoom['_id'][], userId: string) => {
 	const rooms = await Rooms.findByIds(rids).toArray();
 
 	const teamsToRemove = rooms.filter((room) => room.teamMain);
 	const teamIds = teamsToRemove.map((room) => room.teamId).filter((teamId) => teamId !== undefined);
 	const uniqueTeamIds = [...new Set(teamIds)];
+
+	if (!userId) {
+		throw new Error('error-user-not-found');
+	}
 
 	await Promise.all(
 		uniqueTeamIds.map(async (teamId) => {
-			if (!userId) {
-				throw new Error('error-user-not-found');
-			}
-
 			const team = await Team.findOneById(teamId);
 			if (!team) {
 				throw new Error('error-team-not-found');
 			}
 
 			await eraseTeam(userId, team, []);
 		}),
 	);
 };

---

45-45: Prefer cleaner conditional promise pattern.

The spread pattern ...[userId && bulkTeamCleanup(rids, userId)] works (Promise.all handles the false value when userId is falsy), but is not idiomatic.

Apply this diff for clearer intent:

 	await Promise.all([
 		Subscriptions.removeByRoomIds(rids, {
 			async onTrash(doc) {
 				void notifyOnSubscriptionChanged(doc, 'removed');
 			},
 		}),
 		Messages.removeByRoomIds(rids),
 		ReadReceipts.removeByRoomIds(rids),
-		...[userId && bulkTeamCleanup(rids, userId)],
+		...(userId ? [bulkTeamCleanup(rids, userId)] : []),
 	]);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)

44-44: Verify the double-deletion flow is safe.

Based on past review discussions, there's a known issue where the team's main room may be deleted twice:

1. First by eraseTeam (called within bulkTeamCleanup at line 24, which internally calls eraseRoom for the main room)
2. Then by Rooms.removeByIds(rids) at line 44 if the main room ID is in rids

Per previous comments, this is being tracked separately in CORE-1432 and Rooms.removeByIds is expected to be idempotent (won't fail on non-existent IDs). However, since eraseRoom "does a lot more than simply delete from mongo" (past comment), please confirm this flow is safe and won't cause cascading issues.

apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)

41-41: Prefer explicit ternary for conditional Promise inclusion.

The pattern ...[userId && bulkTeamCleanup(rids, userId)] works but relies on spreading a falsy value into the array when userId is undefined. A more explicit and readable approach:

-		...[userId && bulkTeamCleanup(rids, userId)],
+		...(userId ? [bulkTeamCleanup(rids, userId)] : []),

This makes the intent clearer: conditionally add the Promise to the array only when userId is defined.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (2)

10-33: Tighten typing and aggregation in bulkTeamCleanup.

The overall flow (identify teamMain rooms, dedupe teamIds, call eraseTeamOnRelinquishRoomOwnerships, aggregate deleted IDs) looks correct and aligns with the PR goals. Two small improvements:

1. teamIds still has a potential undefined in its type even after filter((teamId) => teamId !== undefined). To keep TypeScript happy and avoid accidentally querying _id: undefined, consider a type‑guard filter:

const teamIds = teamsToRemove
	.map((room) => room.teamId)
	.filter((teamId): teamId is string => typeof teamId === 'string');

2. Since deletedRoomIds is populated from multiple async branches and may also contain rooms beyond the original rids, using a Set<string> here (mirroring eraseTeamOnRelinquishRoomOwnerships) would avoid any accidental duplication:

const deletedRoomIds = new Set<string>();

await Promise.all(
	uniqueTeamIds.map(async (teamId) => {
		const team = await Team.findOneById(teamId);
		if (!team) {
			return;
		}

		const ids = await eraseTeamOnRelinquishRoomOwnerships(team, []);
		ids.forEach((id) => deletedRoomIds.add(id));
	}),
);

return [...deletedRoomIds];

---

35-61: Verify cleanup for rooms deleted only via team erasure and consider minor optimization.

The new bulkRoomCleanUp correctly delegates team‑owned rooms to bulkTeamCleanup first and then runs eraseRoomLooseValidation on the remaining room IDs, returning all deleted IDs. Two follow‑ups:

1. Rooms deleted indirectly via eraseTeamOnRelinquishRoomOwnerships (i.e., team rooms not present in the original rids) will not go through the top‑level FileUpload.removeFilesByRoomId(rid) call on Line 37. This is fine if deleteRoom (invoked by eraseRoomLooseValidation) already takes care of uploads for those rooms; otherwise, uploads for those extra team rooms could be left behind.

   Please confirm whether deleteRoom fully handles file cleanup for any room it deletes. If it does not, you likely need to either:

   • call FileUpload.removeFilesByRoomId inside the eraseTeamOnRelinquishRoomOwnerships callback for each rid it deletes, or
   • after bulkTeamCleanup, invoke removeFilesByRoomId for any deletedRoomIds that are not already in the original rids.

2. Minor: restRidsToRemove is computed via deletedRoomIds.includes(rid), which is O(n²) in the worst case. Since membership is all you need there, you can snapshot the team‑deleted IDs into a Set for that filter:

const deletedByTeam = new Set(deletedRoomIds);
const restRidsToRemove = rids.filter((rid) => !deletedByTeam.has(rid));

The rest of the logic, including returning deletedRoomIds so callers can surface deletedRooms, looks good.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.

Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).

Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.

Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.

Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.